Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

node-api: run Node-API finalizers in GC second pass #42208

Closed
wants to merge 9 commits into from
Closed

node-api: run Node-API finalizers in GC second pass #42208

wants to merge 9 commits into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Mar 4, 2022 •
edited
Loading

The issue

Currently Reference finalizers are run inside of SetImmediate.
In case if user code creates a lot of native objects in the main script, it could cause a significant memory pressure, even if the objects are properly released. This is because they are "collected" only inside of SetImmediate that follows the script run.
See issue: nodejs/node-addon-api#1140

In the a74a6e3 commit the processing of finalizers was moved from the GC second pass to the SetImmediate because finalizers may throw JavaScript exceptions which can affect behavior of other functions.
If the JavaScript exception is thrown inside of SetImmediate, then it causes an unhandled exception which can be handled process-wide with help of process.on('uncaughtException', ...) event handler.

The solution

In this PR we are switching back to processing finalizers in the GC second pass which may happen any time after GC calls.
To address the issue of handling JS exceptions from finalizers we do the following in this PR:

  • By default, all JS exceptions from finalizers cause the Node.js unhandled exceptions. We apply the same error handling mechanism as used by immediate tasks created by SetImmediate. Thus, we address the previous issue with the finalizer JS errors interrupting other functions, and align the error handling behavior with SetImmediate.
  • In addition to it, we are adding new node_api_set_finalizer_error_handler public API to setup an error handler per napi_env instance which is created per each native module. Each native module can handle its finalizer JS errors on its own.

Tests

New test_finalizer_exception test is added to js-native-api to verify the new behavior.
All other js-native-api, node-api, and cctests are passing.

Documentation

The n-api.md documentation is updated with the new node_api_set_finalizer_error_handler public API.
See the n-api.md for the details about the node_api_set_finalizer_error_handler function.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 4, 2022
@vmoroz vmoroz changed the title src: node-api: Implement finalizer_queue to run finalizers earlier node-api: Implement finalizer_queue to run finalizers earlier Mar 4, 2022
@vmoroz vmoroz changed the title node-api: Implement finalizer_queue to run finalizers earlier node-api: implement finalizer_queue to run finalizers earlier Mar 4, 2022
@Trott
Copy link
Member

Trott commented Mar 4, 2022

@nodejs/node-api Does this require any documentation additions or new tests?

@vmoroz
Copy link
Member Author

vmoroz commented Mar 4, 2022 •
edited
Loading

@nodejs/node-api Does this require any documentation additions or new tests?

It is changing the internal behavior - I am not sure about the docs.
Though, we definitely need a new test for it.
I hope to discuss this change with the Node-API team tomorrow and if the overall approach makes sense, then I will add the test and any required docs.

@mhdawson mhdawson added the node-api Issues and PRs related to the Node-API. label Mar 4, 2022
@vmoroz
Copy link
Member Author

vmoroz commented Mar 4, 2022 •
edited
Loading

While I work on the unit test, I see that I was completely wrong about the root cause of the issue.
I thought that the issue was the run of the GC second pass callback, while the true issue is with the Node-API code that always runs finalizers as SetImmediate. See:

node/src/node_api.cc

Lines 36 to 48 in b35ee26

void node_napi_env__::CallFinalizer(napi_finalize cb, void* data, void* hint) {
// we need to keep the env live until the finalizer has been run
// EnvRefHolder provides an exception safe wrapper to Ref and then
// Unref once the lambda is freed
EnvRefHolder liveEnv(static_cast<napi_env>(this));
node_env()->SetImmediate(
[=, liveEnv = std::move(liveEnv)](node::Environment* node_env) {
napi_env env = liveEnv.env();
v8::HandleScope handle_scope(env->isolate);
v8::Context::Scope context_scope(env->context());
env->CallIntoModule([&](napi_env env) { cb(env, data, hint); });
});
}

Converting the status of the PR to a draft for now.

@vmoroz vmoroz marked this pull request as draft March 4, 2022 17:36
@vmoroz
Copy link
Member Author

vmoroz commented Mar 4, 2022 •
edited
Loading

Running all finalizers in the SetImmediate was introduced by this commit: a74a6e3

It sounds like that in order to do it right we must:

  • call finalizers synchronously while draining the finalizing queue
  • stop draining on the first JS exception
  • change the draining of the queue from the GC second pass to still use the SetImmediate to conform the existing behavior
  • have tests for both cases with and without exceptions

@vmoroz
Copy link
Member Author

vmoroz commented Mar 11, 2022

We had discussed this PR in our Node-API meeting today on 3/11/2022. The key takeaways from the discussion:

  • It is a good idea to have a configurable feature that controls whether to drain finalizer queue from Node-API methods that may touch GC or not.
  • Returning JS errors from finalizers as a part of other Node-API method calls does not seem to be right. We must follow the same error reporting pattern as we have today with the use of SetImmediate.
  • The overall approach based on the finalizing queue seems to be promising.

Some thoughts about the finalizer error handling after the meeting:

  • By default, JS errors in finalizers should be treated as unhandled JS exceptions as we do it today.
  • We should add a new public method node_api_set_finalizer_error_handler that can handle finalizer errors per addon.
  • The node_api_call_finalizers should return finalizer error or allow to choose it to be handled by local or global handlers discussed above.
  • Document how JS errors should be handled inside of the finalizer code and what happens when they are not handled.

@vmoroz vmoroz changed the title node-api: implement finalizer_queue to run finalizers earlier node-api: run Node-API finalizers in GC second pass Mar 18, 2022
@vmoroz vmoroz marked this pull request as ready for review March 18, 2022 17:32
@vmoroz
Copy link
Member Author

vmoroz commented Mar 18, 2022

I ended up removing the finalizing_queue in favor of using GC second pass as it used to be before a74a6e3 and fixing the error handing mechanism for finalizers.

This was referenced Mar 18, 2022
Closed
Closed
@vmoroz vmoroz closed this Mar 29, 2022
@vmoroz vmoroz deleted the FinalizerQueue branch March 29, 2022 20:31
@vmoroz
Copy link
Member Author

vmoroz commented Mar 29, 2022

Sorry, I accidently removed the PR branch. The PR is re-created as #42515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax Awaiting requested review from addaleax

@gabrielschulhof gabrielschulhof Awaiting requested review from gabrielschulhof

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vmoroz @nodejs-github-bot @Trott @mhdawson